-
Notifications
You must be signed in to change notification settings - Fork 15.4k
[CIR] Upstream global variable linkage types #129072
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-clangir @llvm/pr-subscribers-clang Author: Morris Hafner (mmha) ChangesThis change implements variable linkage types in ClangIR except for common linkage which requires Comdat support. Patch is 37.19 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/129072.diff 19 Files Affected:
diff --git a/clang/include/clang/CIR/Dialect/IR/CIRDialect.h b/clang/include/clang/CIR/Dialect/IR/CIRDialect.h
index 683176b139ca4..0684cf5034f5d 100644
--- a/clang/include/clang/CIR/Dialect/IR/CIRDialect.h
+++ b/clang/include/clang/CIR/Dialect/IR/CIRDialect.h
@@ -28,6 +28,8 @@
#include "clang/CIR/Dialect/IR/CIRAttrs.h"
#include "clang/CIR/Dialect/IR/CIROpsDialect.h.inc"
+#include "clang/CIR/Dialect/IR/CIROpsEnums.h"
+#include "clang/CIR/Interfaces/CIROpInterfaces.h"
// TableGen'erated files for MLIR dialects require that a macro be defined when
// they are included. GET_OP_CLASSES tells the file to define the classes for
diff --git a/clang/include/clang/CIR/Dialect/IR/CIROps.td b/clang/include/clang/CIR/Dialect/IR/CIROps.td
index f9ce38588e436..7c5d339130ec5 100644
--- a/clang/include/clang/CIR/Dialect/IR/CIROps.td
+++ b/clang/include/clang/CIR/Dialect/IR/CIROps.td
@@ -18,6 +18,8 @@ include "clang/CIR/Dialect/IR/CIRDialect.td"
include "clang/CIR/Dialect/IR/CIRTypes.td"
include "clang/CIR/Dialect/IR/CIRAttrs.td"
+include "clang/CIR/Interfaces/CIROpInterfaces.td"
+
include "mlir/IR/BuiltinAttributeInterfaces.td"
include "mlir/IR/EnumAttr.td"
include "mlir/IR/SymbolInterfaces.td"
@@ -278,6 +280,59 @@ def ScopeOp : CIR_Op<"scope", [
// GlobalOp
//===----------------------------------------------------------------------===//
+// Linkage types. This is currently a replay of llvm/IR/GlobalValue.h, this is
+// currently handy as part of forwarding appropriate linkage types for LLVM
+// lowering, specially useful for C++ support.
+
+// Externally visible function
+def Global_ExternalLinkage :
+ I32EnumAttrCase<"ExternalLinkage", 0, "external">;
+// Available for inspection, not emission.
+def Global_AvailableExternallyLinkage :
+ I32EnumAttrCase<"AvailableExternallyLinkage", 1, "available_externally">;
+// Keep one copy of function when linking (inline)
+def Global_LinkOnceAnyLinkage :
+ I32EnumAttrCase<"LinkOnceAnyLinkage", 2, "linkonce">;
+// Same, but only replaced by something equivalent.
+def Global_LinkOnceODRLinkage :
+ I32EnumAttrCase<"LinkOnceODRLinkage", 3, "linkonce_odr">;
+// Keep one copy of named function when linking (weak)
+def Global_WeakAnyLinkage :
+ I32EnumAttrCase<"WeakAnyLinkage", 4, "weak">;
+// Same, but only replaced by something equivalent.
+def Global_WeakODRLinkage :
+ I32EnumAttrCase<"WeakODRLinkage", 5, "weak_odr">;
+// TODO: should we add something like appending linkage too?
+// Special purpose, only applies to global arrays
+// def Global_AppendingLinkage :
+// I32EnumAttrCase<"AppendingLinkage", 6, "appending">;
+// Rename collisions when linking (static functions).
+def Global_InternalLinkage :
+ I32EnumAttrCase<"InternalLinkage", 7, "internal">;
+// Like Internal, but omit from symbol table, prefix it with
+// "cir_" to prevent clash with MLIR's symbol "private".
+def Global_PrivateLinkage :
+ I32EnumAttrCase<"PrivateLinkage", 8, "cir_private">;
+// ExternalWeak linkage description.
+def Global_ExternalWeakLinkage :
+ I32EnumAttrCase<"ExternalWeakLinkage", 9, "extern_weak">;
+// Tentative definitions.
+def Global_CommonLinkage :
+ I32EnumAttrCase<"CommonLinkage", 10, "common">;
+
+/// An enumeration for the kinds of linkage for global values.
+def GlobalLinkageKind : I32EnumAttr<
+ "GlobalLinkageKind",
+ "Linkage type/kind",
+ [Global_ExternalLinkage, Global_AvailableExternallyLinkage,
+ Global_LinkOnceAnyLinkage, Global_LinkOnceODRLinkage,
+ Global_WeakAnyLinkage, Global_WeakODRLinkage,
+ Global_InternalLinkage, Global_PrivateLinkage,
+ Global_ExternalWeakLinkage, Global_CommonLinkage
+ ]> {
+ let cppNamespace = "::cir";
+}
+
// TODO(CIR): For starters, cir.global has only name and type. The other
// properties of a global variable will be added over time as more of ClangIR
// is upstreamed.
@@ -289,12 +344,19 @@ def GlobalOp : CIR_Op<"global"> {
The backing memory for the variable is allocated statically and is
described by the type of the variable.
+
+ The `linkage` tracks C/C++ linkage types, currently very similar to LLVM's.
}];
- let arguments = (ins SymbolNameAttr:$sym_name, TypeAttr:$sym_type,
- OptionalAttr<AnyAttr>:$initial_value);
+ let arguments = (ins SymbolNameAttr:$sym_name,
+ TypeAttr:$sym_type,
+ Arg<GlobalLinkageKind, "linkage type">:$linkage,
+ OptionalAttr<AnyAttr>:$initial_value,
+ UnitAttr:$dsolocal);
let assemblyFormat = [{
+ $linkage
+ (`dsolocal` $dsolocal^)?
$sym_name
custom<GlobalOpTypeAndInitialValue>($sym_type, $initial_value)
attr-dict
@@ -303,12 +365,25 @@ def GlobalOp : CIR_Op<"global"> {
let extraClassDeclaration = [{
bool isDeclaration() { return !getInitialValue(); }
bool hasInitializer() { return !isDeclaration(); }
+ bool hasAvailableExternallyLinkage() {
+ return cir::isAvailableExternallyLinkage(getLinkage());
+ }
+ bool hasInternalLinkage() {
+ return cir::isInternalLinkage(getLinkage());
+ }
+ /// Whether the definition of this global may be replaced at link time.
+ bool isWeakForLinker() { return cir::isWeakForLinker(getLinkage()); }
+ bool isDSOLocal() { return getDsolocal(); }
}];
let skipDefaultBuilders = 1;
- let builders = [OpBuilder<(ins "llvm::StringRef":$sym_name,
- "mlir::Type":$sym_type)>];
+ let builders = [OpBuilder<(ins
+ "llvm::StringRef":$sym_name,
+ "mlir::Type":$sym_type,
+ // CIR defaults to external linkage.
+ CArg<"cir::GlobalLinkageKind",
+ "cir::GlobalLinkageKind::ExternalLinkage">:$linkage)>];
let hasVerifier = 1;
}
diff --git a/clang/include/clang/CIR/Dialect/IR/CIROpsEnums.h b/clang/include/clang/CIR/Dialect/IR/CIROpsEnums.h
new file mode 100644
index 0000000000000..a1e32209555f3
--- /dev/null
+++ b/clang/include/clang/CIR/Dialect/IR/CIROpsEnums.h
@@ -0,0 +1,119 @@
+//===- CIROpsEnumsDialect.h - MLIR Dialect for CIR ----------------------*- C++
+//-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file declares the Target dialect for CIR in MLIR.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef MLIR_DIALECT_CIR_CIROPSENUMS_H_
+#define MLIR_DIALECT_CIR_CIROPSENUMS_H_
+
+#include "mlir/IR/BuiltinAttributes.h"
+#include "clang/CIR/Dialect/IR/CIROpsEnums.h.inc"
+
+namespace cir {
+
+static bool isExternalLinkage(GlobalLinkageKind linkage) {
+ return linkage == GlobalLinkageKind::ExternalLinkage;
+}
+static bool isAvailableExternallyLinkage(GlobalLinkageKind linkage) {
+ return linkage == GlobalLinkageKind::AvailableExternallyLinkage;
+}
+static bool isLinkOnceAnyLinkage(GlobalLinkageKind linkage) {
+ return linkage == GlobalLinkageKind::LinkOnceAnyLinkage;
+}
+static bool isLinkOnceODRLinkage(GlobalLinkageKind linkage) {
+ return linkage == GlobalLinkageKind::LinkOnceODRLinkage;
+}
+static bool isLinkOnceLinkage(GlobalLinkageKind linkage) {
+ return isLinkOnceAnyLinkage(linkage) || isLinkOnceODRLinkage(linkage);
+}
+static bool isWeakAnyLinkage(GlobalLinkageKind linkage) {
+ return linkage == GlobalLinkageKind::WeakAnyLinkage;
+}
+static bool isWeakODRLinkage(GlobalLinkageKind linkage) {
+ return linkage == GlobalLinkageKind::WeakODRLinkage;
+}
+static bool isWeakLinkage(GlobalLinkageKind linkage) {
+ return isWeakAnyLinkage(linkage) || isWeakODRLinkage(linkage);
+}
+static bool isInternalLinkage(GlobalLinkageKind linkage) {
+ return linkage == GlobalLinkageKind::InternalLinkage;
+}
+static bool isPrivateLinkage(GlobalLinkageKind linkage) {
+ return linkage == GlobalLinkageKind::PrivateLinkage;
+}
+static bool isLocalLinkage(GlobalLinkageKind linkage) {
+ return isInternalLinkage(linkage) || isPrivateLinkage(linkage);
+}
+static bool isExternalWeakLinkage(GlobalLinkageKind linkage) {
+ return linkage == GlobalLinkageKind::ExternalWeakLinkage;
+}
+LLVM_ATTRIBUTE_UNUSED static bool isCommonLinkage(GlobalLinkageKind linkage) {
+ return linkage == GlobalLinkageKind::CommonLinkage;
+}
+LLVM_ATTRIBUTE_UNUSED static bool
+isValidDeclarationLinkage(GlobalLinkageKind linkage) {
+ return isExternalWeakLinkage(linkage) || isExternalLinkage(linkage);
+}
+
+/// Whether the definition of this global may be replaced by something
+/// non-equivalent at link time. For example, if a function has weak linkage
+/// then the code defining it may be replaced by different code.
+LLVM_ATTRIBUTE_UNUSED static bool
+isInterposableLinkage(GlobalLinkageKind linkage) {
+ switch (linkage) {
+ case GlobalLinkageKind::WeakAnyLinkage:
+ case GlobalLinkageKind::LinkOnceAnyLinkage:
+ case GlobalLinkageKind::CommonLinkage:
+ case GlobalLinkageKind::ExternalWeakLinkage:
+ return true;
+
+ case GlobalLinkageKind::AvailableExternallyLinkage:
+ case GlobalLinkageKind::LinkOnceODRLinkage:
+ case GlobalLinkageKind::WeakODRLinkage:
+ // The above three cannot be overridden but can be de-refined.
+
+ case GlobalLinkageKind::ExternalLinkage:
+ case GlobalLinkageKind::InternalLinkage:
+ case GlobalLinkageKind::PrivateLinkage:
+ return false;
+ }
+ llvm_unreachable("Fully covered switch above!");
+}
+
+/// Whether the definition of this global may be discarded if it is not used
+/// in its compilation unit.
+LLVM_ATTRIBUTE_UNUSED static bool
+isDiscardableIfUnused(GlobalLinkageKind linkage) {
+ return isLinkOnceLinkage(linkage) || isLocalLinkage(linkage) ||
+ isAvailableExternallyLinkage(linkage);
+}
+
+/// Whether the definition of this global may be replaced at link time. NB:
+/// Using this method outside of the code generators is almost always a
+/// mistake: when working at the IR level use isInterposable instead as it
+/// knows about ODR semantics.
+LLVM_ATTRIBUTE_UNUSED static bool isWeakForLinker(GlobalLinkageKind linkage) {
+ return linkage == GlobalLinkageKind::WeakAnyLinkage ||
+ linkage == GlobalLinkageKind::WeakODRLinkage ||
+ linkage == GlobalLinkageKind::LinkOnceAnyLinkage ||
+ linkage == GlobalLinkageKind::LinkOnceODRLinkage ||
+ linkage == GlobalLinkageKind::CommonLinkage ||
+ linkage == GlobalLinkageKind::ExternalWeakLinkage;
+}
+
+LLVM_ATTRIBUTE_UNUSED static bool isValidLinkage(GlobalLinkageKind gl) {
+ return isExternalLinkage(gl) || isLocalLinkage(gl) || isWeakLinkage(gl) ||
+ isLinkOnceLinkage(gl);
+}
+
+} // namespace cir
+
+#endif // MLIR_DIALECT_CIR_CIROPSENUMS_H_
diff --git a/clang/include/clang/CIR/Dialect/IR/CMakeLists.txt b/clang/include/clang/CIR/Dialect/IR/CMakeLists.txt
index 1fdbc24ba6b4a..39292fb541daa 100644
--- a/clang/include/clang/CIR/Dialect/IR/CMakeLists.txt
+++ b/clang/include/clang/CIR/Dialect/IR/CMakeLists.txt
@@ -16,4 +16,6 @@ add_dependencies(mlir-headers MLIRCIROpsIncGen)
mlir_tablegen(CIROpsAttributes.h.inc -gen-attrdef-decls)
mlir_tablegen(CIROpsAttributes.cpp.inc -gen-attrdef-defs)
-add_public_tablegen_target(MLIRCIRAttrsEnumsGen)
+mlir_tablegen(CIROpsEnums.h.inc -gen-enum-decls)
+mlir_tablegen(CIROpsEnums.cpp.inc -gen-enum-defs)
+add_public_tablegen_target(MLIRCIREnumsGen)
diff --git a/clang/include/clang/CIR/Interfaces/CIROpInterfaces.h b/clang/include/clang/CIR/Interfaces/CIROpInterfaces.h
new file mode 100644
index 0000000000000..86064619af7db
--- /dev/null
+++ b/clang/include/clang/CIR/Interfaces/CIROpInterfaces.h
@@ -0,0 +1,29 @@
+//===- CIROpInterfaces.h - CIR Op Interfaces --------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef MLIR_INTERFACES_CIR_OP_H_
+#define MLIR_INTERFACES_CIR_OP_H_
+
+#include "mlir/IR/Attributes.h"
+#include "mlir/IR/Operation.h"
+#include "mlir/IR/Value.h"
+#include "mlir/Interfaces/CallInterfaces.h"
+
+#include "clang/AST/Attr.h"
+#include "clang/AST/DeclTemplate.h"
+#include "clang/AST/Mangle.h"
+#include "clang/CIR/Dialect/IR/CIROpsEnums.h"
+
+namespace cir {} // namespace cir
+
+/// Include the generated interface declarations.
+#include "clang/CIR/Interfaces/CIROpInterfaces.h.inc"
+
+namespace cir {} // namespace cir
+
+#endif // MLIR_INTERFACES_CIR_OP_H_
diff --git a/clang/include/clang/CIR/Interfaces/CIROpInterfaces.td b/clang/include/clang/CIR/Interfaces/CIROpInterfaces.td
new file mode 100644
index 0000000000000..b765a3e65f70b
--- /dev/null
+++ b/clang/include/clang/CIR/Interfaces/CIROpInterfaces.td
@@ -0,0 +1,63 @@
+//===- CIROpInterfaces.td - CIR Op Interface Definitions --------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef MLIR_CIR_OP_INTERFACES
+#define MLIR_CIR_OP_INTERFACES
+
+include "mlir/IR/OpBase.td"
+
+let cppNamespace = "::cir" in {
+ def CIRGlobalValueInterface
+ : OpInterface<"CIRGlobalValueInterface"> {
+
+ let methods = [
+ InterfaceMethod<"",
+ "bool", "hasAvailableExternallyLinkage", (ins), [{}],
+ /*defaultImplementation=*/[{ return false; }]
+ >,
+ InterfaceMethod<"",
+ "bool", "hasLocalLinkage", (ins), [{}],
+ /*defaultImplementation=*/[{
+ return cir::isLocalLinkage($_op.getLinkage());
+ }]
+ >,
+ InterfaceMethod<"",
+ "bool", "hasExternalWeakLinkage", (ins), [{}],
+ /*defaultImplementation=*/[{
+ return cir::isExternalWeakLinkage($_op.getLinkage());
+ }]
+ >,
+ InterfaceMethod<"",
+ "bool", "isExternalLinkage", (ins), [{}],
+ /*defaultImplementation=*/[{
+ return cir::isExternalLinkage($_op.getLinkage());
+ }]
+ >,
+ InterfaceMethod<"",
+ "bool", "isDeclarationForLinker", (ins), [{}],
+ /*defaultImplementation=*/[{
+ if ($_op.hasAvailableExternallyLinkage())
+ return true;
+ return $_op.isDeclaration();
+ }]
+ >,
+ InterfaceMethod<"",
+ "void", "setDSOLocal", (ins "bool":$val), [{}],
+ /*defaultImplementation=*/[{
+ $_op.setDsolocal(val);
+ }]
+ >,
+ ];
+ let extraClassDeclaration = [{
+ bool canBenefitFromLocalAlias();
+ }];
+ }
+
+} // namespace cir
+
+#endif // MLIR_CIR_OP_INTERFACES
diff --git a/clang/include/clang/CIR/Interfaces/CMakeLists.txt b/clang/include/clang/CIR/Interfaces/CMakeLists.txt
index 1c90b6b5a23cb..e9929f6964605 100644
--- a/clang/include/clang/CIR/Interfaces/CMakeLists.txt
+++ b/clang/include/clang/CIR/Interfaces/CMakeLists.txt
@@ -3,6 +3,14 @@
# directory which is not the case for CIR (and also FIR, both have similar
# workarounds).
+function(add_clang_mlir_op_interface interface)
+ set(LLVM_TARGET_DEFINITIONS ${interface}.td)
+ mlir_tablegen(${interface}.h.inc -gen-op-interface-decls)
+ mlir_tablegen(${interface}.cpp.inc -gen-op-interface-defs)
+ add_public_tablegen_target(MLIR${interface}IncGen)
+ add_dependencies(mlir-generic-headers MLIR${interface}IncGen)
+endfunction()
+
function(add_clang_mlir_type_interface interface)
set(LLVM_TARGET_DEFINITIONS ${interface}.td)
mlir_tablegen(${interface}.h.inc -gen-type-interface-decls)
@@ -11,4 +19,5 @@ function(add_clang_mlir_type_interface interface)
add_dependencies(mlir-generic-headers MLIR${interface}IncGen)
endfunction()
+add_clang_mlir_op_interface(CIROpInterfaces)
add_clang_mlir_type_interface(CIRFPTypeInterface)
diff --git a/clang/include/clang/CIR/MissingFeatures.h b/clang/include/clang/CIR/MissingFeatures.h
index d4fcd52e7e6e3..0d0083850eb70 100644
--- a/clang/include/clang/CIR/MissingFeatures.h
+++ b/clang/include/clang/CIR/MissingFeatures.h
@@ -36,6 +36,10 @@ struct MissingFeatures {
static bool opGlobalConstant() { return false; }
static bool opGlobalAlignment() { return false; }
static bool opGlobalLinkage() { return false; }
+
+ static bool supportIFuncAttr() { return false; }
+ static bool supportVisibility() { return false; }
+ static bool supportComdat() { return false; }
};
} // namespace cir
diff --git a/clang/lib/CIR/CodeGen/CIRGenModule.cpp b/clang/lib/CIR/CodeGen/CIRGenModule.cpp
index d8acc99e550ad..8eb08228075ef 100644
--- a/clang/lib/CIR/CodeGen/CIRGenModule.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenModule.cpp
@@ -18,6 +18,7 @@
#include "clang/AST/GlobalDecl.h"
#include "clang/Basic/SourceManager.h"
#include "clang/CIR/Dialect/IR/CIRDialect.h"
+#include "clang/CIR/MissingFeatures.h"
#include "mlir/IR/BuiltinOps.h"
#include "mlir/IR/Location.h"
@@ -31,7 +32,7 @@ CIRGenModule::CIRGenModule(mlir::MLIRContext &mlirContext,
const clang::CodeGenOptions &cgo,
DiagnosticsEngine &diags)
: builder(mlirContext, *this), astContext(astContext),
- langOpts(astContext.getLangOpts()),
+ langOpts(astContext.getLangOpts()), codeGenOpts(cgo),
theModule{mlir::ModuleOp::create(mlir::UnknownLoc::get(&mlirContext))},
diags(diags), target(astContext.getTargetInfo()), genTypes(*this) {
@@ -176,6 +177,18 @@ void CIRGenModule::emitGlobalVarDefinition(const clang::VarDecl *vd,
}
varOp.setInitialValueAttr(initializer);
}
+
+ // Set CIR's linkage type as appropriate.
+ cir::GlobalLinkageKind linkage =
+ getCIRLinkageVarDefinition(vd, /*IsConstant=*/false);
+
+ // Set CIR linkage and DLL storage class.
+ varOp.setLinkage(linkage);
+
+ if (linkage == cir::GlobalLinkageKind::CommonLinkage) {
+ errorNYI(initExpr->getSourceRange(), "common linkage");
+ }
+
theModule.push_back(varOp);
} else {
errorNYI(vd->getSourceRange().getBegin(),
@@ -210,6 +223,193 @@ void CIRGenModule::emitGlobalDefinition(clang::GlobalDecl gd,
llvm_unreachable("Invalid argument to CIRGenModule::emitGlobalDefinition");
}
+static bool shouldBeInCOMDAT(CIRGenModule &cgm, const Decl &d) {
+ assert(!cir::MissingFeatures::supportComdat());
+
+ if (d.hasAttr<SelectAnyAttr>())
+ return true;
+
+ GVALinkage linkage;
+ if (auto *vd = dyn_cast<VarDecl>(&d))
+ linkage = cgm.getASTContext().GetGVALinkageForVariable(vd);
+ else
+ linkage =
+ cgm.getASTContext().GetGVALinkageForFunction(cast<FunctionDecl>(&d));
+
+ switch (linkage) {
+ case clang::GVA_Internal:
+ case clang::GVA_AvailableExternally:
+ case clang::GVA_StrongExternal:
+ return false;
+ case clang::GVA_DiscardableODR:
+ case clang::GVA_StrongODR:
+ return true;
+ }
+ llvm_unreachable("No such linkage");
+}
+
+// TODO(CIR): this could be a common method between LLVM codegen.
+static bool isVarDeclStrongDefinition(const ASTContext &astContext,
+ CIRGenModule &cgm, const VarDecl *vd,
+ bool noCommon) {
+ // Don't give variables common linkage if -fno-common was specified unless it
+ // was overridden by a NoCommon attribute.
+ if ((noCommon || vd->hasAttr<NoCommonAttr>()) && !vd->hasAttr<CommonAttr>())
+ return true;
+
+ // C11 6.9.2/2:
+ // A declaration of an identifier for an object that has file scope without
+ // an initializer, and without a storage-class specifier or with the
+ // storage-class specifier static, constitutes a tentative definition.
+ if (vd->getInit() || vd->hasExternalStorage())
+ return true;
+
+ // A variable cannot be both common and exist in a section.
+ if (vd->hasAttr<SectionAttr>())
+ return true;
+
+ // A variable cannot be both common and exist in a section.
+ // We don't try to determine which is the right section in the front-end.
+ // If no specialized section name is applicable, it will resort to default.
+ if (vd->hasAttr<PragmaClangBSSSectionAttr>() ||
+ vd->hasAttr<PragmaClangDataSectionAttr>() ||
+ vd->hasAttr<PragmaClangRelroSectionAttr>() ||
+ vd->hasAttr<PragmaClangRodataSectionAttr>())
+ return true;
+
+ // Thread local vars aren't considered common linkage.
+ if (...
[truncated]
|
erichkeane
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only nits from me. I'll add other reviewers however.
andykaylor
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also update clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp to use the linkage? There are two places marked by cir::MissingFeatures::opGlobalLinkage() that would need to be updated, plus (hopefully) test changes.
It would also be good to have tests for more linkage types. I'm not sure there is a single existing test that verifies these. I found some in clang/test/CIR/CodeGen/weak.c in the incubator and some other examples in clang/test/CodeGen/global-decls.c. Perhaps a new global-linkage.cpp test could be added that cover all the variations we currently support?
| llvm_unreachable("No such linkage"); | ||
| } | ||
|
|
||
| // TODO(CIR): this could be a common method between LLVM codegen. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you do that as part of this change? Maybe move this into ASTContext?
| // See https://llvm.org/LICENSE.txt for license information. | ||
| // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | ||
| // | ||
| //===----------------------------------------------------------------------===// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I notice we're missing file descriptions in header blocks of several of the files you're adding here. I don't know how much people care about that, but the coding standards say there say there should be one. Many existing files that have it don't follow the doxygen format described in the coding standards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great to see this PR Morris! You should also add CIR <-> CIR tests for the linkage types, incubator has some of them you can copy n paste.
dkolsen-pgi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the things I would have commented were already mentioned by someone else. Just a couple small additional items.
| let methods = [ | ||
| InterfaceMethod<"", | ||
| "bool", "hasAvailableExternallyLinkage", (ins), [{}], | ||
| /*defaultImplementation=*/[{ return false; }] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this return false by default? Shouldn't this follow same pattern as other has methods, i.e.:
/*defaultImplementation=*/[{
return cir::isAvailableExternallyLinkage($_op.getLinkage());
}]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed these in incubator: llvm/clangir#1423 and refined some additional class declarations. Changes from that PR can be applied here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I applied your changes in my patch except for the additions to CallOp which doesn't exist, yet.
| } | ||
|
|
||
| // TODO(CIR): this could be a common method between LLVM codegen. | ||
| static bool isVarDeclStrongDefinition(const ASTContext &astContext, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a copy-paste from clang/lib/CodeGen/CodeGenModule.h without an actual dependency on CIR.
CIRGenModule is only used to obtain ASTContext and check cgm.supportsCOMDAT(), both of which can be passed as direct arguments. In fact, ASTContext already is; it just needs to be forwarded to shouldBeInCOMDAT.
Would it be better to expose this function from Clang CodeGen and adjust the interface for use here? What do you think, @erichkeane?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds like a good idea. IMO we should have a directory of common codegen stuff like this that we can use to extract stuff from llvm-ir codegen and use from CIR. This seems like a good candidate to start that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have annotated tons of these functions around, I think they resemble the idea of ASTCodegenHelpers, since they allow AST queries that have no dependency to codegen and are also useful to other tools (each these days implement their own). Not sure where exactly the implementation should live, but it would be nice if those can be exposed on a public header
| return false; | ||
| } | ||
|
|
||
| cir::GlobalLinkageKind CIRGenModule::getCIRLinkageForDeclarator( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To further reduce duplication, why not use an enum conversion like toCirLinkage(llvm::GlobalValue::LinkageTypes linkage) -> cir::GlobalLinkageKind? This would allow most of these functions to be implemented in terms of LLVM CodeGen:
cir::GlobalLinkageKind getCIRLinkageForDeclarator(const DeclaratorDecl *dd, GVALinkage linkage, bool isConstantVariable) {
return toCirLinkage(getLLVMLinkageForDeclarator(...));
}However, this would require changes to the incubator and CodeGen first, as some functions are private methods or rely on langOpts from CodeGen. So this is more of a question for @erichkeane and @bcardosolopes—would this be feasible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is likely a general policy question: how tightly should CodeGens be coupled, and should we modify main CodeGen to better support API reuse for CIRCodeGen? I expect many similar copies, independent of the actual CIR and MLIR, to appear during upstreaming. (Also cc @AaronBallman)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I answered above, but if modifying current CodeGen mildly is necessary in order to make the CIR process easier (and to improve the amount of shared code), then I'm 100% for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this would require changes to the incubator and CodeGen first, as some functions are private methods or rely on langOpts from CodeGen. So this is more of a question for @erichkeane and @bcardosolopes—would this be feasible?
I'm not a big fan of depending on LLVM stuff at the CIR level, given we can get all info we need from the AST and even though it'd be a practical solution - my take is to keep that at the lowering level.
This is likely a general policy question: how tightly should CodeGens be coupled, and should we modify main CodeGen to better support API reuse for CIRCodeGen? I expect many similar copies, independent of the actual CIR and MLIR, to appear during upstreaming.
I believe reuse is always great, but IMO I don't think we need to generate extra work for upstreaming on anything other than things like a ASTCodegenHelpers atm.
b945378 to
881dd98
Compare
erichkeane
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy when everyone else is, the refactor to extract common functions is valuable but can be/should be done in a separate patch.
bcardosolopes
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM too
cac8ff2 to
a07e741
Compare
| InterfaceMethod<"", "bool", "isWeakForLinker", (ins), [{}], | ||
| /*defaultImplementation=*/[{ | ||
| return cir::isWeakForLinker($_op.getLinkage()); | ||
| }]>]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this intended indentation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So to speak, yes. I reformatted the file with clang-format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe that's why clang-format isn't commonly used on table-gen files. That indentation is wrong and makes it (a tiny bit) harder to read.
a07e741 to
f2de49d
Compare
This change implements variable linkage types in ClangIR except for common linkage which requires Comdat support.
- Various formatting changes - Changed file header comments - Changed include guards - Added global linkage to LLVM lowering
- Add header comments to CIROpInterfaces - Fix test clang-cir-ir-global
Co-authored-by: Henrich Lauko <[email protected]>
a2afb02 to
9f4fcdf
Compare
|
Looks like ready to land, @mmha do you have landing permission? |
|
Great! I don't have committer status, yet, if that's your question. |
|
@mmha Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
This change implements variable linkage types in ClangIR except for common linkage which requires Comdat support.